Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Carrying/escape-inventory tweaks #834

Merged
merged 12 commits into from
Feb 15, 2024
Merged

Carrying/escape-inventory tweaks #834

merged 12 commits into from
Feb 15, 2024

Conversation

Mnemotechnician
Copy link
Contributor

@Mnemotechnician Mnemotechnician commented Jan 6, 2024 •
edited
Loading

About the PR

  • Adds a new action that stops the "escape inventory" do-after. The action only appears when you try to struggle out of the inventory, e.g. while being carried or sitting in a bag. Should affect mice and such as well.
  • Fixes dropping the carried person when walking over to a different grid (I can't count how many times I've been dropped when someone tried to carry me out of a shuttle!)
  • Makes sure the carried person always stays at (0, 0) relative to the carrier. It used to be possible to offset the carried person by walking near a singuloose gravitational anomaly or by enclosing the carried person in a container while staying just outside it.

Why / Balance

🥺

Technical details

New action is a bit of C# changes + new yml and texture files

The second change is achieved by changing the OnParentChanged callback to check if the new ParentUid is the same as GridUid, which means the new parent is a grid and not chair/another carrier

The third change is achieved by iterating over all entities with BeingCarriedComponent and making sure their LocalPosion is approximately equal to (0, 0)

Media

The third and second changes should not require media, but here's a showcase of the new action:

weeee-2024-01-06_17.20.18.mp4
  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

None? I think.

Changelog
🆑

  • tweak: You no longer drop the carried person when walking across different grids.
  • add: You can now halt your attempt to escape from a bag or someone's hands.
  • add: Trying to take someone out of a bag now results in carrying them.
  • add: You can now insert carried people into bags.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2024 •
edited
Loading

RSI Diff Bot; head commit dc65e3d merging into deda6cd
This PR makes changes to 1 or more RSIs. Here is a summary of all changes:

Resources/Textures/_NF/Actions/escapeinventory.rsi

State Old New Status
cancel-escape Added

Edit: diff updated after dc65e3d

@Leander-0
Copy link
Contributor

would it not be better to change the do_after so you can escape inventory when pressing the action than just canceling it after moving? sounds more intuitive to me.

@Mnemotechnician
Copy link
Contributor Author

would it not be better to change the do_after so you can escape inventory when pressing the action than just canceling it after moving? sounds more intuitive to me.

I'm not sure, but people are already used to the old system...

@Leander-0
Copy link
Contributor

i mean the old system hasn't been changed since a year or more ago and its not like its a complete change from ground up will be very clear when the action shows to escape since there is little tie to react to the escape do_after to when you want to cancel it, its more easy to just try to escape when you want if you dont try it then it means you dont want.

@Mnemotechnician
Copy link
Contributor Author

I don't want to change things that already work, because if I fuck up while doing it, it will be a much more serious problem than if I introduce a smaller change to fix the issue in hindsight...

@github-actions github-actions bot added the Merge Conflict This PR has conflicts that prevent merging label Jan 20, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the Merge Conflict This PR has conflicts that prevent merging label Jan 20, 2024
@Mnemotechnician
Copy link
Contributor Author

Mnemotechnician commented Jan 20, 2024 •
edited
Loading

More changes:

  • Trying to take someone out of a bag results in you trying to carry them (since this is what picking up someone is meant to do, don't you think?)
  • Added a verb to insert someone you carry into a bag

https://cdn.discordapp.com/attachments/1123826879892299783/1198371899944738946/weeee-2024-01-20_23.58.09.mp4?ex=65bea9cd&is=65ac34cd&hm=90b0efd59d2905d4d81ed43476c1837394e747184f794be44a9ff8ba72172455&

@dvir001
Copy link
Contributor

dvir001 commented Jan 25, 2024

Let me know when this is ready for review.

@Mnemotechnician
Copy link
Contributor Author

Let me know when this is ready for review.

It was ready for review 3 weeks ago.

@Cheackraze
Copy link
Member

sorry for the delay while we handled The Big Merge ™️ , feel free to pull an updated master and I'll give this a full review. I had to put a bit of work into getting PsuedoItem to even work with the new grid inv in the first place, so there may or may not be some additional work needed on this as well

@Mnemotechnician
Copy link
Contributor Author

It's up-to-date now, and it does compile and work. Also fixed another bug introduced by the merge (pressing shift would make you try to escape an inventory)

I was trying to fix throwing carried people, but after an hour of fiddling with the debugger I had no luck so far, I guess it's to be fixed in another pr

@github-actions github-actions bot added the Status: Needs Review This PR is awaiting reviews label Feb 11, 2024
@Cheackraze Cheackraze merged commit 3074b2b into new-frontiers-14:master Feb 15, 2024
14 checks passed
FrontierATC added a commit that referenced this pull request Feb 15, 2024
@FrontierATC
Automatic Changelog (#834)
f034c42
@Mnemotechnician Mnemotechnician mentioned this pull request Apr 21, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cheackraze Cheackraze Cheackraze approved these changes

@FoxxoTrystan FoxxoTrystan FoxxoTrystan approved these changes

Assignees
No one assigned
Labels
C# FTL Sprites Status: Needs Review This PR is awaiting reviews YML
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Mnemotechnician @Leander-0 @dvir001 @Cheackraze @FoxxoTrystan